Improvements over chuking refactoring#4533
Open
h-mayorquin wants to merge 4 commits intoSpikeInterface:mainfrom
Open
Improvements over chuking refactoring#4533h-mayorquin wants to merge 4 commits intoSpikeInterface:mainfrom
h-mayorquin wants to merge 4 commits intoSpikeInterface:mainfrom
Conversation
alejoe91
reviewed
Apr 20, 2026
| def __init__( | ||
| self, | ||
| chunkable: ChunkableMixin, | ||
| chunkable: TimeSeries, |
Member
There was a problem hiding this comment.
Suggested change
| chunkable: TimeSeries, | |
| time_series: TimeSeries, |
?
Member
|
I agree @h-mayorquin should we also rename then the |
Collaborator
Author
Yes, I agree. |
Member
|
In case this takes a some time to merge, I've done a quick dirty patch to fix the kilosort issue here #4540 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes the Kilosort4 regression reported on #4472. The root cause is that #4472 turned
write_binary_recordinginto a plain alias forwrite_binary(chunkable, ...), so passingrecording=recas a keyword argument no longer matched the parameter name and raisedTypeError. The immediate fix is to makewrite_binary_recordinga real wrapper again, withrecordingas its actual parameter:I think this points to a more general principle worth keeping in the codebase: the SpikeInterface public API should stay in SpikeInterface vocabulary. A user saving a recording should see
recordingin signatures, docstrings, and IDE autocomplete, not the implementation conceptchunkable. Keeping the generic functions private (underscore-prefixed, living intime_series_tools) and exposing thin recording-centered wrappers as the public surface is what makes this class of regression structurally hard to reintroduce: the parameter name a user types is the one that lives in the signature, and cannot drift when the internals are refactored.write_memory_recordingis converted the same way for consistency.The second change is the naming of the abstraction introduced in #4472. We touched on this in the maintenance meeting with @alejoe91: the real abstraction here is not chunkable in general, it is a time series over which we run processes in chunks. My proposal is that the class names should reflect that rather than staying at the overly abstract
Chunkablelevel. Concretely:ChunkableMixinis renamed toTimeSeries: what this class actually models is a time series. Slicing methods likeget_data(start_frame, end_frame, ...)are a natural property of a sliceable time series, not an extra "chunkable" capability. The executor does the chunking; the class just has to expose a time-series contract.ChunkableSegmentis renamed toTimeSeriesSegment: the segment class contains no chunking code at all. It is only time handling (sampling_frequency,t_start,time_vector,get_times,sample_index_to_time,time_to_sample_index), and its own docstring describes it as providing "methods to handle time kwargs." DroppingChunkablematches what the class actually is.ChunkExecutoris renamed toTimeSeriesChunkExecutor: this is where the verb "chunk" belongs, because the executor is what performs the chunking. Naming itTimeSeriesChunkExecutoralso makes the axis of chunking explicit (it chunks along time, not arbitrary axes) and ties the vocabulary together: aTimeSeries/TimeSeriesSegmentis chunked by aTimeSeriesChunkExecutor. The modulechunkable_tools.pyand the helperdivide_chunkable_into_chunksare renamed totime_series_tools.pyanddivide_time_series_into_chunksto keep the import path and internal utilities consistent with the new names.The two ideas come together in the zarr writer. Before this PR the public function was literally named
write_chunkable_to_zarr: the function name itself asked a SpikeInterface user to learn the internal concept "chunkable" just to save a recording. Now the public entry point iswrite_recording_to_zarr(recording, ...)and the generic implementation has moved to a private_write_time_series_to_zarr(...), so the helper name is honest about theTimeSeriescontract it operates on. The behavior is identical; only the surface a user sees changes. WhenBaseImaginglands,write_imaging_to_zarr(imaging, ...)will be a second thin wrapper over the same private core, and imaging users will never seechunkableeither.